Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multiple listeners for Virtual Nodes, Routers, and Gateways #27368

Closed
wants to merge 18 commits into from

Conversation

benmurden
Copy link
Contributor

@benmurden benmurden commented Oct 20, 2022

Description

Add multiple listeners to App Mesh in order to support the changes made generally available on Aug 17, 2022.

Relations

Closes #26380

References

https://aws.amazon.com/about-aws/whats-new/2022/08/aws-app-mesh-support-multiple-listeners/

Output from Acceptance Testing

$ make testacc TESTS=TestAccAppMesh_serial/VirtualNode/multipleListeners PKG=appmesh

=== RUN   TestAccAppMesh_serial
=== RUN   TestAccAppMesh_serial/VirtualNode
=== RUN   TestAccAppMesh_serial/VirtualNode/multipleListeners
--- PASS: TestAccAppMesh_serial (14.29s)
    --- PASS: TestAccAppMesh_serial/VirtualNode (14.29s)
        --- PASS: TestAccAppMesh_serial/VirtualNode/multipleListeners (14.29s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/appmesh    14.532s

$ make testacc TESTS=TestAccAppMesh_serial/VirtualRouter/multipleListeners PKG=appmesh

=== RUN   TestAccAppMesh_serial
=== RUN   TestAccAppMesh_serial/VirtualRouter
=== RUN   TestAccAppMesh_serial/VirtualRouter/multipleListeners
--- PASS: TestAccAppMesh_serial (23.05s)
    --- PASS: TestAccAppMesh_serial/VirtualRouter (23.05s)
        --- PASS: TestAccAppMesh_serial/VirtualRouter/multipleListeners (23.05s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/appmesh    23.269s

$ make testacc TESTS=TestAccAppMesh_serial/VirtualGateway/multipleListeners PKG=appmesh

=== RUN   TestAccAppMesh_serial
=== RUN   TestAccAppMesh_serial/VirtualGateway
=== RUN   TestAccAppMesh_serial/VirtualGateway/multipleListeners
--- PASS: TestAccAppMesh_serial (23.02s)
    --- PASS: TestAccAppMesh_serial/VirtualGateway (23.02s)
        --- PASS: TestAccAppMesh_serial/VirtualGateway/multipleListeners (23.02s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/appmesh    23.230s

$ make testacc TESTS=TestAccAppMesh_serial/GatewayRoute/(grpcRoute|httpRoute|http2Route) PKG=appmesh

=== RUN   TestAccAppMesh_serial
=== RUN   TestAccAppMesh_serial/GatewayRoute
=== RUN   TestAccAppMesh_serial/GatewayRoute/grpcRoute
=== RUN   TestAccAppMesh_serial/GatewayRoute/httpRoute
=== RUN   TestAccAppMesh_serial/GatewayRoute/http2Route
--- PASS: TestAccAppMesh_serial (323.53s)
    --- PASS: TestAccAppMesh_serial/GatewayRoute (323.53s)
        --- PASS: TestAccAppMesh_serial/GatewayRoute/grpcRoute (83.68s)
        --- PASS: TestAccAppMesh_serial/GatewayRoute/httpRoute (120.19s)
        --- PASS: TestAccAppMesh_serial/GatewayRoute/http2Route (119.66s)
PASS

$ make testacc TESTS=TestAccAppMesh_serial/Route/(grpc|http2?|tcp)RouteTargetPort PKG=appmesh

=== RUN   TestAccAppMesh_serial
=== RUN   TestAccAppMesh_serial/GatewayRoute
=== RUN   TestAccAppMesh_serial/Route
=== RUN   TestAccAppMesh_serial/Route/httpRouteTargetPort
=== RUN   TestAccAppMesh_serial/Route/tcpRouteTargetPort
=== RUN   TestAccAppMesh_serial/Route/grpcRouteTargetPort
=== RUN   TestAccAppMesh_serial/Route/http2RouteTargetPort
=== RUN   TestAccAppMesh_serial/VirtualRouter
--- PASS: TestAccAppMesh_serial (125.41s)
    --- PASS: TestAccAppMesh_serial/GatewayRoute (0.00s)
    --- PASS: TestAccAppMesh_serial/Route (125.40s)
        --- PASS: TestAccAppMesh_serial/Route/httpRouteTargetPort (33.03s)
        --- PASS: TestAccAppMesh_serial/Route/tcpRouteTargetPort (31.33s)
        --- PASS: TestAccAppMesh_serial/Route/grpcRouteTargetPort (30.70s)
        --- PASS: TestAccAppMesh_serial/Route/http2RouteTargetPort (30.34s)
    --- PASS: TestAccAppMesh_serial/VirtualRouter (0.00s)
PASS

@github-actions
Copy link

Community Note

Voting for Prioritization

  • Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
  • Please see our prioritization guide for information on how we prioritize.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

  • Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@github-actions github-actions bot added needs-triage Waiting for first response or review from a maintainer. service/appmesh Issues and PRs that pertain to the appmesh service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/XL Managed by automation to categorize the size of a PR. labels Oct 20, 2022
@benmurden benmurden marked this pull request as draft October 20, 2022 22:19
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome @benmurden 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTOR guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

"spec.0.listener.0.connection_pool.0.http",
"spec.0.listener.0.connection_pool.0.http2",
"spec.0.listener.0.connection_pool.0.tcp",
},
Copy link
Contributor Author

@benmurden benmurden Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are no longer singleton items, so this would throw an error. Not sure of a way to keep these checks while adding the ability to specify multiple listeners. Advice welcome here.

ExactlyOneOf: file configuration
block reference (spec.0.listener.0.connection_pool.0.grpc) can only
be used with TypeList and MaxItems: 1 configuration blocks

@github-actions
Copy link

Thank you for your contribution! 🚀

Please note that the CHANGELOG.md file contents are handled by the maintainers during merge. This is to prevent pull request merge conflicts, especially for contributions which may not be merged immediately. Please see the Contributing Guide for additional pull request review items.

Remove any changes to the CHANGELOG.md file and commit them in this pull request to prevent delays with reviewing and potentially merging this pull request.

@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. labels Oct 21, 2022
@benmurden benmurden marked this pull request as ready for review October 21, 2022 03:56
@github-actions github-actions bot removed the provider Pertains to the provider itself, rather than any interaction with AWS. label Oct 21, 2022
@benmurden benmurden changed the title Add multiple listeners for Virtual Nodes Add multiple listeners for Virtual Nodes, Routers, and Gateways Oct 21, 2022
@justinretzolk justinretzolk added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Oct 21, 2022
@SW386
Copy link

SW386 commented Oct 31, 2022

Wouldn't we also need to enable specifying port match in route and gateway route? AWS throws errors if you attempt to create routes to gateways/routers/nodes with multiple listeners without those specifications.

@benmurden
Copy link
Contributor Author

@SW386 Thanks for the comment. Do you have an example of a config that would now fail as a result of these changes?

@SW386
Copy link

SW386 commented Nov 1, 2022

I do not have anything I can share, but If you go to the AWS Appmesh console and have a route defined without a port match connected to a node, then try to modify that node to add multiple listeners, you will receive All Routes that are targeting this Virtual Node must define a port before adding multiple listeners to this Virtual Node. Same thing happens with multiple listeners on the Router and Gateway. Terraform currently doesn't support specifying this property.

@benmurden
Copy link
Contributor Author

I see, thanks for clarifying. Are there examples of Terraform resources that validate based on the content of other resources? It seems to me this sort of thing is usually covered by documentation e.g. Security Groups and Security Group Rules https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule.

@SW386
Copy link

SW386 commented Nov 2, 2022

I don't have anything on the Terraform side since those resources are not updated to include port match and multiple listeners are not supported. However, on the AWS side, you can find the dependencies here:

Virtual Routes:
https://docs.aws.amazon.com/app-mesh/latest/userguide/routes.html

Virtual Gateway Rotues:
https://docs.aws.amazon.com/app-mesh/latest/userguide/gateway-routes.html

For both resources, under match configuration, port match is optional for a single listener but required for multiple listeners.

@benmurden
Copy link
Contributor Author

benmurden commented Nov 2, 2022

I think I see now. Target Port is a required attribute when the target Virtual Node has multiple listeners, but is not yet in the schema. 🤔

OK, let me see if I can add Target Port as well.

@benmurden
Copy link
Contributor Author

@SW386 I think that should do it for adding target ports. Could you have a look and confirm everything looks to be in order?

@SW386
Copy link

SW386 commented Nov 15, 2022

This looks right to me!

@benmurden
Copy link
Contributor Author

Seems another PR #27799 was merged instead. Closing.

@benmurden benmurden closed this Jan 20, 2023
@benmurden benmurden deleted the app-mesh-multiple-listeners branch January 20, 2023 00:25
@benmurden benmurden restored the app-mesh-multiple-listeners branch January 20, 2023 00:25
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/appmesh Issues and PRs that pertain to the appmesh service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_appmesh_virtual_node should support multiple listeners
3 participants